avoid closing over values in string eval when comparing values #1018
+12
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The test script for this could be patched to try running with Devel::Cover, but it's rather ugly. Here's a patch that adds this: I can add this to the PR if desired. It would also need to install Devel::Cover during the automated testing, which has its own complications. |
|
I agree the test is ugly, and we can skip it. However I would like a comment added to the patch explaining why we do the sub wrapping, that will help avoid repeating the bug later by any future contributors. |
When comparing values using Test::Builder::cmp_ok, a string eval is used to match the caller location, and to inject an arbitrary comparison op. When used with Devel::Cover, the string eval may close over any variables used in it, and maintain them for the lifetime of the program. Instead of comparing the values directly in the string eval, generate an anonymous sub that will perform the comparison, and call it in a block eval. This is a reoccurrance of a previous problem that was fixed with 4fe707c. Some combination of perl or module updates has brought back the issue. While there was a test included with the bug, it only fails when run with Devel::Cover, which isn't done during the automated or release testing.
haarg
force-pushed
the
no-capture-cmp_ok
branch
from
cc69866 to
a49f733
Compare
|
Updated to include a comment. |
exodist
added a commit
that referenced
this pull request
Jan 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When comparing values using Test::Builder::cmp_ok, a string eval is used to match the caller location, and to inject an arbitrary comparison op. When used with Devel::Cover, the string eval may close over any variables used in it, and maintain them for the lifetime of the program.
Instead of comparing the values directly in the string eval, generate an anonymous sub that will perform the comparison, and call it in a block eval.
This is a reoccurrance of a previous problem that was fixed with 4fe707c. Some combination of perl or module updates has brought back the issue. While there was a test included with the bug, it only fails when run with Devel::Cover, which isn't done during the automated or release testing.